Repository navigation
[openapi3] emit additionalProperties for a declared Record indexer in 3.1 - #11954
Conversation
… 3.1 A declared `Record<T>` indexer describes a dictionary. Sealing, and a model that declares an indexer while also extending another model, both have to account for the properties evaluated by the `allOf` subschema holding the base model, so those keep `unevaluatedProperties`. Everything else now emits `additionalProperties`. Measured with Ajv 2020-12: in the position this changes the two keywords accept and reject the same instances; in the position it excludes they differ, and `additionalProperties` would make the schema unsatisfiable. A dictionary that does not compose now emits the same schema in 3.0 and 3.1, which the new cross-version tests use as an oracle, with the two exclusions pinned in the other direction.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates OpenAPI 3.1/3.2 emission so non-composing Record<T> schemas use converter-compatible additionalProperties.
Changes:
- Selects
additionalPropertiesorunevaluatedPropertiesbased on sealing and inheritance. - Adds extensive cross-version, inheritance, sealing, nullable, and versioning tests.
- Updates documentation and changelog metadata.
File summaries
| File | Description |
|---|---|
| website/src/content/docs/docs/getting-started/typespec-for-openapi-dev.md | Updated as part of this pull request. |
| packages/openapi3/test/works-for.ts | Updated as part of this pull request. |
| packages/openapi3/test/versioning.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/test-host.ts | Updated as part of this pull request. |
| packages/openapi3/test/return-types.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/record.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/nullable-properties.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/additional-properties.test.ts | Updated as part of this pull request. |
| packages/openapi3/src/schema-emitter-3-1.ts | Updated as part of this pull request. |
| .chronus/changes/openapi3-record-additional-properties-2026-8-15-19-45-0.md | Updated as part of this pull request. |
Review details
Suppressed comments (3)
.chronus/changes/openapi3-record-additional-properties-2026-8-15-19-45-0.md:2
featureis not the appropriate change kind for this patch: it corrects existingRecord<T>emission for a supported input rather than adding a new capability. Please classify it asfix; that also avoids the feature-only requirement for an illustrative code block.
changeKind: feature
website/src/content/docs/docs/getting-started/typespec-for-openapi-dev.md:539
- This note now says that sealed schemas in OpenAPI 3.1/3.2 use
unevaluatedProperties, but theRecord<never>example immediately below still presentsadditionalPropertiesas the result; that example is wrong for the versions described here. Please qualify the examples as OpenAPI 3.0 or show the version-specific output.
**Note:** when emitting Open API 3.1 and 3.2 specs, `unevaluatedProperties` is used instead of `additionalProperties` for schemas that compose with `allOf`: sealed schemas, and models that declare a `Record` indexer while also extending another model. `additionalProperties` cannot see the properties evaluated by the `allOf` subschema, so it would constrain the inherited ones.
website/src/content/docs/docs/getting-started/typespec-for-openapi-dev.md:539
- This wording is inaccurate for a leaf sealed schema:
seal-object-schemas: trueemitsunevaluatedProperties: { not: {} }even when the schema has noallOf(as covered inpackages/openapi3/test/additional-properties.test.ts:271-280). Separate the sealing case from theallOfrationale so the docs do not imply that composition is required.
**Note:** when emitting Open API 3.1 and 3.2 specs, `unevaluatedProperties` is used instead of `additionalProperties` for schemas that compose with `allOf`: sealed schemas, and models that declare a `Record` indexer while also extending another model. `additionalProperties` cannot see the properties evaluated by the `allOf` subschema, so it would constrain the inherited ones.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi @Zach Bimson (@bimsonz). Your PR has had no update for 14 days and it is marked as a stale PR. If it is not updated within 14 additional days, the PR will automatically be closed. If you want to refresh the PR, please remove the |
|
Hey, thanks for this — we're hitting exactly this right now. We generate a Go server from TypeSpec → OpenAPI 3.1.0 (oapi-codegen on the backend). It just ignores The PR itself looks good — no conflicts, CI is green, and the validation-equivalence table is convincing for the cases it touches. I think it just needs a maintainer's eyes. Timothee Guerin (@timotheeguerin) Alexandre Kohler (@kwy404) could someone take a look? |
commit: |
|
All changed packages have been documented.
Show changes
|
@typespec/openapi3 1.17.0 publishes a leaf dictionary as additionalProperties itself (microsoft/typespec#11954), which is exactly what the shipped patch did. Both patch files and the patchedDependencies entry are gone, and the peer floor is 1.17.0: without the patch the dictionary claim in the guide would be false on 1.15 and 1.16, and 0.5.0 still serves those with its patches. The versioning peer moves to ^0.87.0, the version 1.17.0 pairs with, which the old range excluded. The guide's removal check was wrong for 1.17.0. It said the indexerKeyword grep should fail once the patch was removed, but upstream merged the same identifier, so the grep still matches and a successful removal read as a failed one. The guide now says to read the document, with openapi-typescript 7.13.0's output measured on both versions. patch-effect.test.ts becomes dictionary-keyword.test.ts with its arms unchanged. They read the published document, so they now grade openapi3 itself. Control: openapi3 pinned back to 1.16.0, unpatched, turns the leaf and typed arms red.
…e place Moves every @typespec/* devDependency to 1.17.0 / 0.87.0 together and the corpus to @typespec/http-specs 0.1.0-alpha.44, the release paired with them. The optional streams and versioning peers now admit ^0.87.0, so a consumer on the current compiler no longer gets an unmet peer and, under npm, a second versioning copy. pnpm dedupe leaves one compiler in the tree. 1.17.0 ships microsoft/typespec#11954: a leaf model's declared indexer is published as additionalProperties. opennessOfDocument read only unevaluatedProperties, so every leaf dictionary in the corpus became an "openness" divergence against a validator that agreed with the document. Two other sites already read both keywords. All three now call one reader, extraPropertyDeclaration, and the boolean forms are read as sealed and open. Everything else that moved is the corpus growing, each named: - operations 641 -> 650: response/body-or-no-content (2) and seven new streaming/sse Protocol operations; parity with the document is unchanged. - stream-item bodies 3 -> 10, now asserted as the ten positions rather than a count, read off the scenario's own SSEStream operations. - responseBodyKindsCompared 45 -> 52: the same seven SSE bodies at 3.1. - unprobed components 75 -> 76: Protocol.Data.DataEvents, an anyOf of event variants like the two already counted. - baseline emitterWarnings gains payload/xml truncated-doc-comment, which the suite already expects; the file was last regenerated before that warning existed. hookTimeout gets the headroom testTimeout already had: emit.test.ts's beforeAll takes 4.4s alone and hit the 10s default inside the full run.
What this changes
For OpenAPI 3.1 and 3.2, a declared
Record<T>indexer is emitted asadditionalPropertiesinsteadof
unevaluatedProperties, unless the model also extends another model, or the schema is sealed.Those two keep
unevaluatedProperties, because both have to account for the properties evaluated bythe sibling
allOfsubschema.model.baseModelis the same conditionmodelDeclarationuses to attach theallOf, so the guardand the thing it guards against are decided by the same fact.
3.0 is untouched:
schema-emitter-3-0.tshas noapplyModelIndexerand uses the baseimplementation, which already writes
additionalProperties. 3.2 is affected, becauseschema-emitter-3-2.tsextendsOpenAPI31SchemaEmitterwithout overriding the method, and thetests run against both.
Note
Record<never>is unaffected:shouldSealSchemareturns true for it, so it takes the sealingbranch and keeps
unevaluatedProperties: { not: {} }.Why: this repo cannot read its own 3.1 output
tsp-openapi3, the converter in this package, reads onlyadditionalProperties. Of the 36.tsfiles under
packages/openapi3/src/cli, zero mentionunevaluatedPropertiesand four handleadditionalProperties(convert/generators/generate-model.ts:359,convert/generators/generate-types.ts:482,convert/transforms/transform-component-schemas.ts:129,convert/interfaces.ts).So compiling
model Labels is Record<string>;at 3.1 and converting the result back givesmodel Labels {}. At 3.0 it givesmodel Labels { ...Record<string>; }. The round trip loses everydictionary, and exits 0 while doing it. I have filed that separately, since it stands on its own
whichever way this PR goes.
Why not just keep
unevaluatedPropertiesI am not claiming the current behaviour is accidental. #5961 chose it on purpose and said why:
That rationale is about composition, and it is correct. The keyword was then applied to every
declared indexer, including models that compose with nothing. This narrows it to the cases the
rationale covers.
Is it validation-equivalent where it changes?
Ajv 8.20.0 on the 2020-12 dialect, the same schema pair under each keyword, in every position this
emitter can put it:
$refd inside a derived model'sallOfanyOfbranchallOf(the case this excludes)The last row is why the guard exists. With
model Ledger extends Base { ...Record<string>; }andBasedeclaringid: int32,additionalPropertiesconstrains the inheritedidand the schemarejects
{"id": 1}outright.The second row is why the guard is
model.baseModeland not also aderivedModelscheck.Annotations flow bottom-up, so a dictionary in branch position inside someone else's
allOfbehavesidentically under both keywords. A
derivedModelsguard would also have been non-local, becauseRecord<T>is a cached template instantiation shared by every use site in a program.Coverage, and how strong it is
Because the two keywords are equivalent for a non-composing, non-
neverdictionary, the 3.0 documentis an oracle for the 3.1 one: the two spec versions must produce identical schemas.
record.test.tsasserts that, and asserts the two exclusions (composing models, and
Record<never>) in the otherdirection, so the oracle's preconditions are pinned rather than assumed.
14 tests added. I checked they are not vacuous by mutating the production code three ways and
recording which arms go red:
unevaluatedProperties(full revert)|| model.baseModelarmadditionalPropertiesEvery one of the 14 is killed by at least one. An earlier draft of this branch had a test asserting
that plain arrays are unaffected; it survived all three mutants, because arrays route through
arrayDeclaration/arrayLiteraland never reachapplyModelIndexerat all. I removed it ratherthan keep a test that cannot fail.
Shapes covered: declared indexer alone, with a base model, and as the base of other models; sealed
leaf and sealed with a base;
Record<never>; a three-level chain where two levels declare anindexer; arrays of dictionaries; nested dictionaries; cross-version parity in both directions; and
versioning, where the emitter mutates and clones the model graph, so a clone that lost its
baseModelwould silently change the keyword between two versions of the same API.openApiForVersionscould not pass emitter options, so it takes them now.Verified and unchanged, so not given new tests: merge-patch, multipart, discriminated unions, XML,
@visibilityvariants, and circular models.Blast radius
packages/openapi3: 2602 tests before, 2626 after.pnpm regen-samplesandpnpm regen-specsboth produce zero diff, but I want to be straight aboutwhat that is worth: not much. All 30 checked-in sample outputs are
openapi: 3.0.0and no samplesets
openapi-versions, so the 3.1 emitter is not exercised by them; andregen-specsrecords theconverter snapshots, which are the opposite direction. There is no checked-in 3.1 golden output in
the repo, so the unit tests above are the real coverage. The upside is that this change causes no
golden-file churn for reviewers to read.
Worth noting that
tsp initdoes offer an "OpenAPI 3.1 document" template, so zero golden-file churnis not the same as zero user impact.
One case where this narrows behaviour
@extensionwrites arbitrary keys straight onto the schema, afterapplyModelIndexerhas run. Thex-prefix convention is documented for@extensionbut not enforced, so a spec can inject anin-place applicator that the keyword decision structurally cannot see:
The injected
allOfnow sits besideadditionalProperties, and an instance{"flag": true}thatvalidated before does not after. This also reproduces via
@extensionon a property.I have pinned the current output in a test rather than guarded it, because the guard would have to
run before the extension is attached and cannot. If you would prefer
applyModelIndexerto consultgetExtensionsfor applicator keys, say so and I will add it, but it would only cover the modelpath and not the property one, which is why I did not do it unilaterally.
Why
packages/json-schemais not in this PR#5961 changed both emitters in one changeset, so leaving one behind is a fair thing to ask about. I
looked, and a mechanical port would make things worse rather than better:
Record<never>throughshouldSealSchema/isNeverType.json-schemahas nosuch check and reaches
{ not: {} }through the indexer path instead. Applying the same guardthere would flip
Record<never>toadditionalPropertiesinjson-schemawhile openapi3 keepsunevaluatedProperties, creating a divergence rather than removing one.json-schemadecides itsallOfwithmodel.baseModel && !shouldInlineBase, where the secondterm covers discriminated-union inlining that openapi3 does not have.
So aligning the two is a decision about both emitters and I would rather you made it than have me
assume it. Happy to extend this PR, or follow up separately, whichever you prefer.
Notes
feature, matching OpenAPI/JsonSchema - use unevaluatedProperties instead of additionalProperties and support setting to false #5961, which shipped the original change that way.openapi-typescriptalso does not implementunevaluatedPropertiesand renders a dictionary asRecord<string, never>, discarding the declared value type. That is tracked on their side asSupport for
unevaluatedPropertiesopenapi-ts/openapi-typescript#2839 and is a real gap there, not only here. I mention it as evidenceabout ecosystem support rather than as a defect report against this repo.
and I will rework it. The diff is one method, so either is cheap.